fix(desktop): resolve bundled sidecar on cheap path and bound login-shell spawns - #6904
Conversation
9ad9212 to
8ed97e8
Compare
|
@wpfleger96 may be related to #6872 I see that a lot (and not a slow machine though) |
|
@michaelneale nice thanks for that link, yeah it looks like probably the same issue |
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review pinned to head f42ea036f36a360f606a9d0400ec4d428d8ba026 and base cdab765748ddfeba10830a9a5331b6157bd321df. Product contract: discovery must not fabricate “not installed” from cold state, and every discovery process must have bounded lifetime/tree teardown across supported desktop platforms. I reviewed the boot warmup, cheap/forced catalog paths, bundled-sidecar resolution, login-shell/auth probes, Unix process groups, Windows Job Objects, output capture, and relevant failure/timeout/success transitions. No PR code was executed.
Blocking findings
-
P1 — Windows still leaks descendants when Job Object ownership cannot be established.
BoundedChild::spawnacceptscreate_job_for_child(...) == Noneand continues withjob: None;kill_treethen calls onlyChild::kill(). On the successful fast-child path, the leader has already exited before teardown, so that call cannot reach a backgrounded descendant. This is not merely hypothetical:create_job_for_childperformsOpenProcessafter spawn and explicitly documents the race window; it can also fail for ordinary Job assignment/setup errors. The helper nevertheless claims hard tree termination on every platform and uses this path for login-shell and auth discovery. Fix this by failing the bounded operation closed when tree ownership cannot be established, or by providing a fallback that can still terminate the descendant tree after leader exit. Add Windows lifecycle coverage for assignment failure and success/timeout descendant cleanup; current tests are Unix-only. Anchors:bounded_command.rs#L44-L75,bounded_command.rs#L98-L120,process_lifecycle.rs#L43-L105. -
P2 — The fire-and-forget boot warmup still exposes a cold partial catalog as authoritative “ready.” The AppShell effect starts the forced probe but does not gate or annotate the independent cheap query used by create/edit/defaults. During the documented 20–65 second forced probe, that cheap query can return
NotInstalledfor PATH-only runtimes and consumers mark the catalogready; if the forced warm fails,refreshAcpRuntimesswallows the error and leaves that partial catalog indefinitely. The bundled sidecar stat fix is sound, but this does not satisfy the broader PR/issue claim that installed runtimes stop transiently disappearing across those surfaces. Track the initial forced attempt in shared machine-level state and have hot surfaces remain loading/revalidating (or explicitly degraded) until it settles, preserving existing rows; expose a retryable failure instead of silently blessing cold misses. Add a lifecycle test covering the cheap/forced race and failure state. Anchors:useAppShellLifecycleEffects.ts#L28-L40,hooks.ts#L206-L226,useAgentManagement.ts#L303-L308,acpRuntimesQuery.ts#L41-L67.
Secondary lifecycle note: the new effect is “once per AppShell mount,” not once per process launch, so sequential AppShell remounts can repeat the expensive forced pipeline. A machine-level initial-warm owner/dedupe would address this with finding 2.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review pinned to exact head 0788c3a80ce0531c6b81d1b55d8d2862ec114a1a and base cdab765748ddfeba10830a9a5331b6157bd321df. The prior Windows descendant-ownership and cold-catalog gating blockers are structurally addressed, but the new bounded-command path still has one hard resource-bound defect. I reviewed discovery callers, Windows suspended-spawn/Job ownership and failure cleanup, Unix/Windows teardown, output capture, boot warm state, cheap/forced races, remount/retry behavior, create/edit/defaults consumers, tests, and exact-head CI. No PR code was executed.
Blocking finding
- P1 — Discovery output is still unbounded, so a “bounded” probe can exhaust disk/memory and return well after its deadline.
output_with_timeoutredirects arbitrary login-shell and auth-probe stdout/stderr into unconstrained temp files. During each 10-second process window, a noisy or hostile command can write until the filesystem fills; after teardown,read_capturedperformsread_to_endon both files with no byte ceiling, allocating the complete payload outside the timeout. The process lifetime is bounded, but the operation and its resource use are not. Add a strict per-stream or aggregate capture limit enforced while the child runs, fail closed when it is exceeded, and read at most that limit. Cover an over-limit producer without first materializing its full output. Anchors:bounded_command.rs#L187-L263,login_shell.rs#L45-L69,discovery.rs#L837-L862.
Also fix before re-review
refreshAcpRuntimeswrites the forced catalog before cancelling the in-flight cheap query. TanStack Query cancellation defaults torevert: true; cancelling can restore the cheap query’s pre-fetch state after the forcedsetQueryData, and the code then marks boot warmsettled, exposing that stale/cold state as authoritative. Cancel and await the shared cheap query before writing the forced result and settling the gate. Add a race regression that begins from a pre-existing cold catalog, starts a delayed cheap fetch, completes the forced warm, and proves the shared cache remains forced after cancellation. Anchor:acpRuntimesQuery.ts#L188-L207.
Exact-head CI is otherwise green for Desktop, Desktop E2E, Rust lint/unit/security, and Windows Rust. The failing stale-review workflow is process-only and does not alter this code verdict.
|
Confirmed on 0.5.19 macOS x86_64 with The sidecar cheap-path change would not fix this by itself ( Details on #6662. |
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review pinned to exact head 21372ec6a04ca5e16e1b5876f0cb20fa8bbbb8fa and exact base cdab765748ddfeba10830a9a5331b6157bd321df. Product contract: cold runtime state must not be presented as authoritative, runtime capability remains Rust-catalog-owned, and discovery must terminate with bounded process/resource use. I traced the three save-blocking cheap consumers, forced Settings/onboarding consumers, shared query writes, sidecar packaging/resolution on Unix and Windows, login-shell/auth callers, cache invalidation, process-tree teardown, and exact-head CI. No PR code was checked out or executed.
Blocking finding
- P1 — A stale in-flight login-shell probe can overwrite a newer refresh, recaching a false-negative PATH for the session.
login_shell_path()checksUninit, releases the mutex while it runs an independently fallible 10-second probe, then unconditionally stores its result.refresh_login_shell_path()merely writesUninit; it does not invalidate the writeback rights of an older probe. A concrete interleaving is: probe A starts; a forced refresh resets the cache and probe B starts; B succeeds and stores the fresh PATH; A times out afterward and storesNone. Native agent, model, readiness, and install paths then consumeNoneuntil another explicit refresh, recreating the installed-runtime false negatives this PR is meant to remove. Concurrent cold callers can likewise race success against timeout because each probes independently, so the comment that last-writer-wins is safe because results are identical is false once probes have bounded failure modes. The new automatic boot warm increases the chance of overlap with startup/runtime activity.
Make cache publication generation-aware so a pre-refresh probe cannot commit after invalidation, and single-flight each generation (or otherwise prevent same-generation divergent probes from racing). Add deterministic overlap coverage for refresh-during-probe and timeout-versus-success. Anchors: login_shell.rs#L123-L155, independently fallible bounded probes at login_shell.rs#L41-L72, and forced invalidation before discovery at forced_single_flight.rs#L64-L76.
The prior Windows ownership, output-bound, cheap/forced cache-ordering, and boot-gate blockers are structurally addressed. Sidecar lookup remains filesystem-only and matches the packaged Unix/Windows layout. Non-blocking coverage gap: the new sidecar test uses an explicit temporary absolute path rather than exercising bare buzz-agent through the packaged/current-executable search directories. Exact-head CI is green across Desktop, Desktop E2E, Rust/unit/security, macOS build, and Windows Rust; the Codex security-review job was cancelled, and the Windows lifecycle fixtures remain ignored, so neither is runtime proof of those Windows fixtures.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review pinned to exact head 3234eb6ed8004e77239be48f9f882d50bf66ea48 and exact base 953a1798b23b04289b3a225f463e60a172c1aac3. The prior generation-overwrite blocker is narrowed but not closed, and the bounded-command implementation now directly documents and tests a Unix process-tree escape. I traced login-shell cache publication, forced discovery/cache settlement, Unix process containment, drain termination, and exact-head CI. No PR code was checked out or executed.
Blocking findings
-
P1 — Unix discovery knowingly leaves escaped descendants running after the bounded operation returns.
BoundedChildowns only the child's process group, andkill_treeuseskillpg; a descendant can callsetsid/setpgidand leave that group. The implementation explicitly permits that survivor, despite promising ownership and hard termination of the whole descendant tree on every exit path. The regression test makes this non-theoretical: it launches a descendant that escapes, sleeps for 300 seconds, asserts the PID is still alive afteroutput_with_timeoutreturns, and then manually SIGKILLs it. A login-shell rc file or auth probe can therefore leave arbitrary work consuming resources after discovery. Use a containment boundary a child cannot voluntarily leave, eliminate arbitrary login-shell execution from discovery, or narrow the contract and otherwise guarantee leaked probe descendants cannot survive. Anchors:bounded_command.rs#L76-L99,bounded_command.rs#L340-L362,bounded_command.rs#L660-L714. -
P1 — rejected login-shell probe results are still returned to callers, so a concurrent timeout can publish a false-negative runtime catalog. Two cold callers in generation G probe independently. If B succeeds first, it caches
/fresh/bin; if A times out later,publish_probe_resultcorrectly refuses to overwrite that success withNone, butlogin_shell_pathstill returns A's localNone. A forced discovery using A's result can then write a PATH-missing catalog and mark the UI warm/settled even though the authoritative native cache holds B's successful PATH. A pre-refresh probe likewise returns its stale local result even when generation guarding rejects its writeback. Make each generation genuinely single-flight, or return/retry against the authoritative current-generation value after publication. Cover overlappinglogin_shell_path()return values, not only direct cache-publication assertions. Anchor:login_shell.rs#L138-L183.
Substantive exact-head checks are green, including Desktop, Desktop E2E, Rust lint/unit/security, macOS build, and Windows Rust. The cancelled Codex security-review job is process metadata, not the basis for this verdict.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review pinned to exact head 564edee497eb7503d190ef9c3f5cf3644830d4dc and exact base 953a1798b23b04289b3a225f463e60a172c1aac3. The prior login-shell race blocker is resolved: callers now return the authoritative current-generation cache value after publication and retry when refresh invalidates an in-flight generation, with deterministic return-value tests for both overlap cases. The Unix containment asymmetry is now documented consistently with the adjudicated contract: Unix bounds helper lifetime, capture, and in-group teardown without promising to reap a descendant that voluntarily escapes the process group; Windows retains hard non-breakaway Job Object containment.
I traced cheap/forced discovery, boot-warm and retry state, shared-query write ordering, runtime catalog consumers, login-shell generation transitions, Unix timeout/capture/drain behavior, and Windows spawn/assignment/resume/cleanup paths. No new blocking defect was found. Substantive exact-head checks are green, including Desktop, Desktop E2E, Rust lint/unit/security, macOS build, and Windows Rust; the Codex security-review workflow is still in progress and is process metadata rather than the basis for this source verdict. Review was read-only; no PR code was checked out or executed.
…hell spawns ## Why PR #6330 split harness/runtime detection into a cheap (cache-only) path and a forced (spawning) path. Two regressions followed, both reported as every harness showing "(not installed)" across the create/edit picker, Agents > Agent defaults, and Settings > Agents — blocking agent save until the user clicked Install: - The resolve cache is in-memory, so it starts cold every launch. The cheap path consulted only the managed-shim dirs plus that cold cache, and the managed-shim allowlist structurally excludes `buzz-agent`. The bundled sidecar could therefore never resolve on the cheap path until a forced pass warmed the cache, so cheap-path surfaces rendered all-missing at boot. - `run_in_login_shell` used an untimeouted `Command::output()`; a wedged login shell froze the whole forced pipeline, leaving "Check again" spinning forever. ## What - `resolve_command_cached` now also calls `resolve_workspace_command`, resolving the bundled sidecar via a filesystem stat (no spawn) — the same class of work the managed-shim check already performs. `buzz-agent` can no longer show missing, even during the boot warm window. - Extract `output_with_timeout`, a shared piped/background-drained/SIGTERM-on- deadline spawn helper; refactor `probe_auth_status` onto it. - Bound each login-shell candidate with a 10s timeout via that helper, falling through to the next candidate on timeout instead of aborting the resolve. - Warm the ACP runtime catalog once at AppShell mount so the shared React Query cache is populated before the cheap-path surfaces render. Regression tests cover cold-cache sidecar resolution and the timeout helper's success and kill-on-deadline paths. Origin: Buzz channel harness-detection thread (#5ef5d5bb-643f-4b87-bbf4-e8b64585ffeb). Signed-off-by: Will Pfleger <pfleger.will@gmail.com> Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
…ildren ## Why Review found `output_with_timeout` could still hang indefinitely: it sent only `SIGTERM` to the direct child (no-op on Windows, ignorable on Unix) and then unconditionally joined the wait thread and both pipe-drain threads. A child that traps `SIGTERM`, or a forked descendant that keeps the stdout/stderr pipes open after the direct child exits, left a join blocked forever — recreating the "Check again" spins-forever failure this work removes. ## What Extract the helper into `discovery/bounded_command.rs` and rebuild it on the repo's existing bounded-process pattern (the codex `--version` probe): - Capture stdout/stderr to regular temp files instead of pipes. A regular file returns EOF at its write position regardless of who inherits the descriptor, so a pipe-retaining descendant can no longer block the post-exit read. There are no drain threads to join. - Poll the child with `try_wait` against the deadline instead of a wait thread. - On timeout, spawn the child in its own process group and tear the group down with `SIGTERM`, a bounded grace, then an escalating `SIGKILL` (Unix), or `Child::kill` (Windows) — guaranteed termination, not signal cooperation. Extraction also returns `discovery.rs` (1438 -> 1380) and `discovery/tests.rs` (1820 -> 1820) to within the file-size ratchet, which had been skipping the Rust/unit/desktop-core/Windows CI lanes. Adversarial tests (a `SIGTERM`-ignoring child and a descendant retaining the output descriptors, each with an outer wall-clock bound) verify the hard deadline; the sidecar cheap-path regression moves to the path-resolution suite. Origin: Buzz channel harness-detection thread (#5ef5d5bb-643f-4b87-bbf4-e8b64585ffeb). Signed-off-by: Will Pfleger <pfleger.will@gmail.com> Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
The bounded-command helper only tore down the process group on timeout or spawn error. On a successful exit it reaped the leader and returned, leaving any descendant the child had backgrounded (a login-shell rc worker, an auth CLI daemon) alive with the captured-output descriptors and whatever it was writing. Windows used a direct Child::kill that never touched descendants. Kill the group/tree unconditionally after the leader exits, errors, or times out, before reading captured output. The timeout path keeps its SIGTERM + grace before the final SIGKILL; every other path goes straight to the forced group kill. Windows delegates to taskkill_tree (taskkill /T /F), matching the probe_node discipline already in the tree. Strengthen the adversarial tests: each now runs the helper under an independent wall-clock watchdog thread (a hung helper fails the test instead of hanging it) and asserts the backgrounded descendant is dead after both successful return and timeout. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Two pass-3 review findings. (1) Windows success-path tree leak. taskkill /T /PID is a live-root lookup: once the direct child has exited (the success and post-SIGKILL paths), there is no root for it to enumerate, so a backgrounded descendant orphaned. Take a kill-on-close Job Object at spawn and hold it across the whole bounded operation, mirroring the harness worker-reaping discipline in process_lifecycle.rs. Closing the job reaps the tree even after the root exits. Job assignment failure degrades to killing the direct child. The spawn/wait/kill lifecycle is wrapped in a BoundedChild guard so both platforms own the tree rather than looking it up after the fact. (2) Vacuous descendant assertions. The Unix fixtures recorded $$ inside a ( ... ) subshell, which in /bin/sh is the invoking shell PID, not the background child — so both tests asserted the already-reaped leader was dead, not the descendant. Record the real background PID via $!. Verified non-vacuous by mutation: neutering kill_tree now fails reaps_backgrounded_descendant_on_success (descendant survives). Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The Job Object was assigned after a runnable spawn, so a probe that backgrounded a descendant and exited in the same tick escaped ownership, and job-assignment failure ran the command unowned to the deadline with only a direct-child kill at teardown — the module's hard-tree guarantee was false on both paths. Spawn the child CREATE_SUSPENDED, assign it to the kill-on-close job while frozen, then resume it via a ToolHelp thread snapshot. No descendant can exist until the job owns the root. Any failure to create, assign, or resume is fail-closed: terminate + reap the child and return None, which discovery treats as command-not-found. The degraded job: None teardown branch is now unreachable by construction. Add ignore-gated Windows tests covering the spawn/assign race (looped) and the timeout path, asserting the recorded descendant PID is reaped. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The shared runtime catalog is in-memory and starts cold every launch, so the cheap discovery path reports every harness (not installed) until a forced pass warms it. The create/edit picker and Agents > Agent defaults surfaces derive their status straight from that cheap path, so during the 20-65s boot probe they presented the cold catalog as authoritative — blessing all harnesses unavailable and blocking save — and, if the forced warm failed, held that state indefinitely with no visible retry. Track the initial forced pass in module-level state (idle -> pending -> settled/failed), deduped per launch so AppShell remounts never re-fire it. useAcpRuntimesQuery overlays that gate: a cold catalog reads as loading while pending and as a retryable error carrying the probe's real reason when the warm failed, while a non-empty catalog always wins so a revalidation or later failure never blanks a good list. The gate only overlays once the warm has started, so onboarding (which renders before AppShell fires it) is unaffected; any forced success settles it. Covered by unit tests for the pure gate (pending/failed/idle/settled, catalog-wins) and the startBootWarm failure->retry->settle lifecycle including the no-re-fire guard. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Command::creation_flags replaces rather than ORs, and BoundedChild::spawn is the last writer before spawn, so the caller's earlier configure_no_window (CREATE_NO_WINDOW) was wiped by the bare CREATE_SUSPENDED — Windows login-shell and auth probes could flash a console window during GUI discovery. Spawn with CREATE_SUSPENDED | CREATE_NO_WINDOW owned in one place (BOUNDED_CREATION_FLAGS), drop the now-clobbered configure_no_window calls on the two bounded callers, and add a compile-time guard asserting both bits are always present so a future edit can't silently reopen either the console-flash or the spawn-to-assign race. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The cold cheap discovery response is never empty — it always emits the known runtimes as not_installed/cli_missing rows plus presets. applyBootWarmGate's data.length > 0 early-return therefore bypassed the overlay for the exact payload it exists to gate, so cheap consumers still blessed the cold catalog as authoritative while the first forced pass was in flight and offered no retry when it failed. Gate on the boot-warm state instead: pending overlays loading, failed overlays a retryable error carrying the probe's real reason, idle/settled pass through; query.data is preserved on every branch. Add a useRetryBootWarm hook and wire an in-place Try again affordance into the create/edit picker and Agent defaults (replacing the non-retryable Restart the app copy for the catalog case). Rewrite the regression to model a realistic non-empty cold catalog and assert neither pending nor failed reads as ready. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…ixtures Two correctness gaps on the discovery bounded-command path, both scoped to `bounded_command.rs`: Capture was unbounded. `output_with_timeout` redirected login-shell and auth-probe output into temp files with no ceiling, so a noisy or hostile probe could fill the disk across its 10s window and `read_captured` then allocated the whole payload at read time. The process lifetime was bounded but its resource use was not. Enforce an aggregate 1 MiB `CAPTURE_LIMIT`: check the on-disk size while the child runs (fail closed, kill the tree, return `None`) and again after a within-deadline exit for a burst between polls, and cap `read_captured` with `take` so the buffer can never balloon even if the file grew. Windows ownership fixtures were self-racing. The success/timeout tests exited or blocked the cmd root in the same tick the descendant was spawned, so the kill-on-close job reaped the descendant mid-cold-start before PowerShell could record its PID — the reap working instantly starved the test of its evidence (the `descendant never recorded its PID` failure on a real Windows box). The success test now blocks the root on a synchronous waiter until the PID file is non-empty; both raise the deadline well above a PowerShell cold start. The guarantee under test is unchanged — the descendant is still born inside the job and must be reaped after the root exits. Added Unix regressions for the capture bound (over-limit producer fails closed; under-limit output returns in full). Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…d catalog `refreshAcpRuntimes` wrote the forced catalog with `setQueryData` and only then cancelled the in-flight cheap query on the shared key. An in-flight cheap request that resolves after the write clobbers the fresh forced catalog, and the boot-warm gate then settles on that stale state. Cancel and await the shared cheap query first so the forced `setQueryData` is the final write. Add a regression that holds a real cheap observer fetching, runs the forced refresh, then resolves the cheap request late; the shared cache must remain the forced result and the gate must settle on it. Removing the `cancelQueries` call fails the test, so the cancel is proven load-bearing. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The capture ceiling was enforced by a 50ms metadata().len() poll of a temp file — detection after overrun, bounded by throughput not by the cap, and captured_len mapped a stat failure to 0, failing open. Replace it with two pipe-drain threads that read into buffers capped by a shared aggregate AtomicU64 budget: the moment a bounded read crosses the cap the overflow flag is set, the poll loop kills the already-owned tree, and the probe fails closed. Nothing over the ceiling is ever materialized, and a read error fails closed rather than open. Pipes were abandoned for temp files in an earlier round because a descendant holding the write end could hang the read forever. The fix is a strict kill-before-join ordering: the tree is torn down (Unix process group / Windows kill-on-close Job Object) before any drain is joined, so every writer is dead and each read hits EOF. The capture test now uses an indefinite producer (cat /dev/zero) with timeout >> watchdog, proving the cap — not the deadline — ends the probe. The Windows timeout fixture now waits synchronously on the descendant PID file before its long block, so the reap can never starve the assertion of the PID on a slow host. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…eath The round-8 drains read each pipe to EOF and were joined only after kill_tree, on the premise that teardown kills every writer. On Unix kill_tree is a killpg on the child's process group, so a descendant that calls setsid()/setpgid() while retaining the inherited stdout survives the kill, holds the write end open, and the blocking read never reaches EOF — hanging the join and breaking the hard wall-clock bound (the exact group-escape primitive Thufir reproduced). Make the Unix read ends nonblocking and add a shared stop flag that teardown raises after kill_tree. A killed in-group writer's pipe still reaches EOF and ends its drain; a group-escaping writer never will, so on the next WouldBlock after stop the drain returns instead of parking on it. The escaped writer is allowed to survive — bounded return no longer depends on every inherited writer exiting. The aggregate fail-closed cap is unchanged. Windows is unaffected: its kill-on-close Job Object is created without breakaway, so no writer escapes and the blocking read to job-close EOF stays sound; the stop flag is inert there. Adds a Unix regression whose descendant setsid()s out of the group while holding stdout; the helper must return within the watchdog and fail closed, and the descendant is asserted alive afterward so the return is proven to come from the stop path, not an incidental EOF. Corrects the docs that overclaimed "every writer is dead" to the narrower group contract already stated for BoundedChild. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The round-9 drain consulted the stop flag only after a WouldBlock read. A group-escaped descendant that keeps the pipe continuously readable returns Ok(n) on every read, so the drain never reached that check and the post-teardown join was unbounded again. The overflow branch made it worse by deliberately reading on past the cap. Bound the Ok(n) path directly: the moment a read crosses the aggregate cap, set overflow and return from the drain immediately. A writer that keeps the pipe continuously readable must outpace the drain and so crosses the finite 1 MiB budget in bounded bytes; a writer that dribbles under the drain rate produces WouldBlock, where the stop check already bounds it. A normal child under the cap still drains to EOF untouched. Returning early re-permits a writer blocking on a full pipe, which is fine only because the poll loop then sees overflow and kills the tree — documented at the return site so it is not "fixed" back into an unbounded drain. Adds a deterministic seam regression: a continuously-ready Read impl with stop preset must still complete via the overflow return, with no process or scheduler dependence. Windows fixtures: the inline `-Command` payloads were mangled crossing the Rust-std → cmd.exe → powershell quoting gauntlet and never executed (root exited without running its payload; both ignored tests passed vacuously on a real host). Rewrite them to write each payload to a temp .ps1 invoked via `powershell -ExecutionPolicy Bypass -File`, with a PowerShell root and a `Start-Process -WindowStyle Hidden` descendant so no cmd tokenizer is involved. Keep the PID-file evidence gating and make every assert dump the PowerShell transcripts so a remote failure is self-diagnosing. Test-only; the reaping guarantee under test is unchanged. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…ws lifecycle tests The outer test watchdog panicked inside recv_timeout with no access to the PowerShell transcripts, so a post-timeout teardown/drain hang on the real Windows box would return as a blind panic — the single failure mode these --ignored lifecycle tests exist to diagnose. Split the driver into run_watchdogged_raw returning Result<Option<Output>, WatchdogTimeout> so each call site attaches its own diagnostics; both Windows fixtures now fold dump_logs into their expiry panic. A #[cfg(unix)] run_watchdogged wrapper keeps the Unix fixtures' ergonomics and avoids a dead-code warning on Windows. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
A login-shell PATH probe drops the mutex before spawning and committed its result unconditionally, so a slow pre-refresh probe could overwrite the fresh PATH a post-refresh probe had already cached — recaching a false-negative and reporting harnesses as not-installed until the next explicit refresh. Same- generation cold callers could also let a timeout clobber a peer's success. Add a monotonic generation to the cache: refresh_login_shell_path bumps it, and a probe may only publish for the generation it started under. Within a generation a failure/timeout never overwrites a committed success. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The two race tests publish fixture PATH values into the process-global login-shell cache. Without a trailing refresh the value survives the test and refresh_login_shell_path_clears_cache reads it as its pre-refresh baseline, failing when the fixture PATH differs from a real login-shell probe. Reset the cache to Uninit on exit so sibling tests re-probe. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The stale probe published None, which the same-generation success-retention rule rejects independently of the generation comparison — so the test stayed green even with the generation guard deleted (verified by mutation). Publish a stale success (/stale/bin) instead: only the generation guard can reject it, so deleting the guard now overwrites the fresh PATH and the test fails. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
login_shell_path() published its probe result but then returned its own local value, so a caller whose probe timed out could return None while a peer's success was already cached, and a generation-rejected probe could return a stale value. Re-read the cache under the publish lock and return the committed value; re-probe when a mid-probe refresh invalidated the generation. Adds a cfg(test) probe seam and two return-value race tests. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…tract The BoundedChild header and the output_with_timeout bullet promised hard whole-tree termination on every exit path unconditionally. On Unix a descendant that setsid/setpgids out while holding a pipe can survive one probe; it can never hang or unbound the helper (nonblocking drains + stop flag) and the regression test asserts that survivor. Windows remains a hard whole-tree guarantee (kill-on-close Job Object, no breakaway). State the asymmetry as the adjudicated design. Comment/doc lines only. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
564edee to
51efce3
Compare
* origin/main: fix(desktop): resolve bundled sidecar on cheap path and bound login-shell spawns (#6904) perf(mobile): reduce cold startup and channel rendering delays (#6996) feat(mobile): push notifications MVP (#6269) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…enericize * origin/main: fix(desktop): resolve bundled sidecar on cheap path and bound login-shell spawns (#6904) perf(mobile): reduce cold startup and channel rendering delays (#6996) feat(mobile): push notifications MVP (#6269) refactor(db): extract domain stores from database runtime (#6987) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…agent-edit * origin/main: fix(desktop): resolve bundled sidecar on cheap path and bound login-shell spawns (#6904) perf(mobile): reduce cold startup and channel rendering delays (#6996) feat(mobile): push notifications MVP (#6269) refactor(db): extract domain stores from database runtime (#6987) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…age-rw * origin/main: fix(desktop): resolve bundled sidecar on cheap path and bound login-shell spawns (#6904) perf(mobile): reduce cold startup and channel rendering delays (#6996) feat(mobile): push notifications MVP (#6269) refactor(db): extract domain stores from database runtime (#6987) feat(desktop): add team sharing to community catalog (#3995) Refresh mobile utility surfaces and theme picker (#6944) fix(desktop): complete project empty and context states (#6980) Fix mobile jump-to-latest flicker (#6807) refactor(relay): NIP-98 admin auth with Operator/Moderator roles and NIP-11 discovery (#3777) refactor(db): split channel membership store (#6782) feat(auth): add NIP-FI canonical assertion verifier and contracts (#6776) Signed-off-by: Joel Robotham <jrobotham@squareup.com>
Why
PR #6330 split agent harness/runtime detection into a cheap (cache-only) path and a forced (spawning) path. Two regressions followed, both surfacing as every harness showing "(not installed)" / "CLI missing" across the agent create/edit picker, Agents > Agent defaults, and Settings > Agents — blocking agent create/edit until the user clicked Install in Settings > Agents.
Root cause
One underlying bug, two victims:
resolve_command_cached(the cheap path) consulted only the Buzz-managed shim dirs plus that cold cache, andbuzz_managed_command_path's allowlist structurally excludesbuzz-agent. The bundled sidecar could therefore never resolve on the cheap path until a forced pass warmed the cache, so cheap-path surfaces rendered all-missing at boot. App setup never warms the cache.run_in_login_shellused an untimeoutedCommand::output(); a wedged login shell froze the whole forced pipeline, leaving "Check again" spinning forever.What
resolve_command_cachednow also callsresolve_workspace_command, resolving the bundled sidecar via a filesystem stat (no spawn) — the same class of work the managed-shim check already performs.buzz-agentcan no longer report missing, even inside the boot warm window.discovery/bounded_command.rsruns any discovery child under a hard wall-clock deadline, polling withtry_waitrather than blocking onwait(). Stdout and stderr are piped to two drain threads whose buffers share an aggregateCAPTURE_LIMIT; a breach fails closed (kill the tree, returnNone), so a noisy or hostile probe can force neither unbounded memory nor disk fill. Tree teardown runs on every exit path — timeout, error, cap breach, and success — because a login-shell rc file or auth CLI can legitimately background a descendant that would otherwise outlive discovery. Ownership is deliberately asymmetric:process_group(0)); teardown isSIGTERM→ bounded grace →SIGKILLon the group. A descendant that leaves the group (setsid/setpgid) while holding a pipe is not owned and may survive one probe, but can never hang or unbound the helper: the Unix drains read nonblocking and end onWouldBlockonce teardown sets the stop flag, so the join returns promptly without waiting on an escaped writer's EOF.CREATE_SUSPENDED, assigned to a kill-on-close Job Object while frozen, then resumed. The job owns the root before any descendant can exist and is created without breakaway, so no writer can escape — a hard whole-tree guarantee, and closing the job reaps the tree even after the root has exited. Any failure to create, assign, or resume is fail-closed: the child is terminated and reaped and the spawn returnsNone(discovery treats it as command-not-found) rather than running unowned.AppShellmount and gate the cheap-path surfaces on that pass. A module-level boot-warm state (idle→pending→settled/failed, deduped per launch) letsuseAcpRuntimesQuerypresent a cold catalog as loading while the first forced pass runs and as a retryable error (carrying the probe's real reason) if it fails, instead of blessing "every harness not installed" as authoritative. A non-empty catalog always wins, so a revalidation or later failure never blanks a good list; the gate only overlays once the warm has started, so onboarding (which renders before the warm) is unaffected. Deduping per launch also fixes the previous per-remount re-fire.Verification
Unix teardown and the drain contract are runtime-proven by
#[ignore]-free tests that record a backgrounded descendant's real PID and assert the helper returns promptly on both the success and timeout paths without blocking on that writer. The generation-aware login-shell cache is covered by deterministic tests through acfg(test)injectable probe seam that assert the function's return value under both concurrent-refresh interleavings — the losing caller returns the peer's committed success, and a mid-probe refresh forces a re-probe to the fresh value. The Windows ownership contract has no CI lane, sobounded_command.rscarries two#[ignore]-gated tests (spawn/assign race, looped; and the timeout path) for a sanctioned run on a Windows host. The boot-warm gate is covered by unit tests for the pure overlay and thestartBootWarmfailure → retry → settle lifecycle.Origin: Buzz thread
Fixes #6872
Related #6662